fix: harden Studio IME composer sends#5327
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08de258657
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request introduces IME (Input Method Editor) support to prevent accidental message submission while users are composing text in languages like Japanese, Chinese, or Korean. The changes include adding the @assistant-ui/tap dependency and implementing composition event handlers in both the thread.tsx and shared-composer.tsx components. Feedback highlights that the useImeComposerInputHandlers hook uses a ref for composition state, which fails to trigger re-renders for visual UI updates like disabling the send button. Additionally, the use of e.preventDefault() in onChange handlers is flagged as unusual and potentially disruptive. Finally, a suggestion was made to use the modern e.isComposing property on synthetic events instead of checking the native event or deprecated key codes.
|
tested and working as expected |
Adds dir="auto" to the main, edit, and compare chat composers so RTL scripts (Arabic, Hebrew, Persian, Urdu) flow right to left without forcing the rest of the UI into RTL. Wires a model-free Playwright smoke (multilingual paste round trip across 31 scripts + a stuck-IME composition repro for issue #5318 / PR #5327) into the Studio UI CI job as a third Studio boot, plus a pure-Python static-guard test that locks down dir="auto" on all three composers and the minimal env contract for the smoke.
Adds dir="auto" to the main, edit, and compare chat composers so RTL scripts (Arabic, Hebrew, Persian, Urdu) flow right to left without forcing the rest of the UI into RTL. Wires a model-free Playwright smoke (multilingual paste round trip across 31 scripts + a stuck-IME composition repro for issue #5318 / PR #5327) into the Studio UI CI job as a third Studio boot, plus a pure-Python static-guard test that locks down dir="auto" on all three composers and the minimal env contract for the smoke.
…5485) Adds dir="auto" to the main, edit, and compare chat composers so RTL scripts (Arabic, Hebrew, Persian, Urdu) flow right to left without forcing the rest of the UI into RTL. Wires a model-free Playwright smoke (multilingual paste round trip across 31 scripts + a stuck-IME composition repro for issue #5318 / PR #5327) into the Studio UI CI job as a third Studio boot, plus a pure-Python static-guard test that locks down dir="auto" on all three composers and the minimal env contract for the smoke.
…5551) * studio/chat: release stuck IME flag when compositionend never fires Chrome on Windows talking to a WSL-hosted Studio (issue #5546) fires compositionstart + compositionupdate but no compositionend after the IME commits. The earlier hardening in #5327 cleared the stale flag on the next non-composing input event, which never arrives in this sequence, so composingRef stays true forever and the Send button stays disabled even though the committed CJK text is already in the textarea. Add a watchdog in both useImeComposerInputHandlers (main + edit composer) and SharedComposer (compare mode) that runs the same reset the missing compositionend would have done. The timer is rearmed on every compositionupdate and on every non-composing input so it only fires when the IME pipeline has actually gone quiet — normal candidate selection keeps it alive, the WSL stuck case lets it expire. Extends the existing IME Playwright smoke with a stuck-compositionend repro and adds a static guard so the watchdog can't be removed without the regression tests catching it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * studio/chat: re-pin composing flag on IME keydown to close #5546 watchdog gap The stuck-compositionend watchdog (PR #5551) releases composingRef after 2500 ms of IME silence so Send unwedges in the WSL+Chrome case. The same release also fires during a long candidate-window pause in healthy IMEs, which lets a subsequent IME-confirm Enter slip preedit text through handleSubmit (main composer) or click-Send through send() (compare composer). Add a keydown gate to both composers: when the browser still reports nativeEvent.isComposing or keyCode 229, re-pin composingRef and cancel any pending watchdog so the next form-submit / send() guard refuses. The Send button stays visually enabled (avoids re-introducing the stuck-UI bug) but the submit path is blocked until a real compositionend or non-composing input arrives. Mirrors the existing isComposing guard shape in shared-composer.onKeyDown. Tests: - tests/studio/test_composer_rtl_bidi_attribute.py: two new static guards asserting the keydown gate wiring in both composer files. - tests/studio/playwright_chat_ime_i18n.py: new section 6c repro that fires the IME-confirm keydown after the watchdog has cleared, then triggers form.requestSubmit() and asserts the preedit text is not cleared (would indicate a leaked submit). Verified across Chromium / Firefox / WebKit via a side-by-side pre-PR vs post-PR simulation (54 scenarios, zero pageerror or console.error). The #5546 stuck-end repro still passes (Send re-enables 2.5-3 s after the silent commit) and the new keydown-repin probe confirms the submit gate refuses on all three engines. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * studio/chat: re-arm IME watchdog after keydown re-pin (Codex P1) The keydown re-pin added in 2c3c979 closed the watchdog-race for healthy IMEs, but on the same WSL+Chrome no-compositionend path this PR targets it would re-lock Send permanently: setting composingRef=true and only *clearing* the watchdog leaves the flag pinned forever if no follow-up compositionend or non-composing input ever arrives. Swap clearStuckTimer/clearStuckImeTimer for refreshStuckTimer/ refreshStuckImeTimer in both composer keydown gates so the watchdog fires once more after every IME keypress. Same visual contract — Send stays enabled — the submit gate just keeps a 2.5s window before re-releasing instead of staying locked. Extends the playwright IME smoke with section 6d: clears composing via the watchdog, fires an IME keydown, then waits past the re-armed watchdog window and asserts the form submit actually flushes the textarea. Two new static guards in test_composer_rtl_bidi_attribute lock the refresh call into both keydown handlers. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>
…nslothai#5551) * studio/chat: release stuck IME flag when compositionend never fires Chrome on Windows talking to a WSL-hosted Studio (issue unslothai#5546) fires compositionstart + compositionupdate but no compositionend after the IME commits. The earlier hardening in unslothai#5327 cleared the stale flag on the next non-composing input event, which never arrives in this sequence, so composingRef stays true forever and the Send button stays disabled even though the committed CJK text is already in the textarea. Add a watchdog in both useImeComposerInputHandlers (main + edit composer) and SharedComposer (compare mode) that runs the same reset the missing compositionend would have done. The timer is rearmed on every compositionupdate and on every non-composing input so it only fires when the IME pipeline has actually gone quiet — normal candidate selection keeps it alive, the WSL stuck case lets it expire. Extends the existing IME Playwright smoke with a stuck-compositionend repro and adds a static guard so the watchdog can't be removed without the regression tests catching it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * studio/chat: re-pin composing flag on IME keydown to close unslothai#5546 watchdog gap The stuck-compositionend watchdog (PR unslothai#5551) releases composingRef after 2500 ms of IME silence so Send unwedges in the WSL+Chrome case. The same release also fires during a long candidate-window pause in healthy IMEs, which lets a subsequent IME-confirm Enter slip preedit text through handleSubmit (main composer) or click-Send through send() (compare composer). Add a keydown gate to both composers: when the browser still reports nativeEvent.isComposing or keyCode 229, re-pin composingRef and cancel any pending watchdog so the next form-submit / send() guard refuses. The Send button stays visually enabled (avoids re-introducing the stuck-UI bug) but the submit path is blocked until a real compositionend or non-composing input arrives. Mirrors the existing isComposing guard shape in shared-composer.onKeyDown. Tests: - tests/studio/test_composer_rtl_bidi_attribute.py: two new static guards asserting the keydown gate wiring in both composer files. - tests/studio/playwright_chat_ime_i18n.py: new section 6c repro that fires the IME-confirm keydown after the watchdog has cleared, then triggers form.requestSubmit() and asserts the preedit text is not cleared (would indicate a leaked submit). Verified across Chromium / Firefox / WebKit via a side-by-side pre-PR vs post-PR simulation (54 scenarios, zero pageerror or console.error). The unslothai#5546 stuck-end repro still passes (Send re-enables 2.5-3 s after the silent commit) and the new keydown-repin probe confirms the submit gate refuses on all three engines. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * studio/chat: re-arm IME watchdog after keydown re-pin (Codex P1) The keydown re-pin added in 2c3c979 closed the watchdog-race for healthy IMEs, but on the same WSL+Chrome no-compositionend path this PR targets it would re-lock Send permanently: setting composingRef=true and only *clearing* the watchdog leaves the flag pinned forever if no follow-up compositionend or non-composing input ever arrives. Swap clearStuckTimer/clearStuckImeTimer for refreshStuckTimer/ refreshStuckImeTimer in both composer keydown gates so the watchdog fires once more after every IME keypress. Same visual contract — Send stays enabled — the submit gate just keeps a 2.5s window before re-releasing instead of staying locked. Extends the playwright IME smoke with section 6d: clears composing via the watchdog, fires an IME keydown, then waits past the re-armed watchdog window and asserts the form submit actually flushes the textarea. Two new static guards in test_composer_rtl_bidi_attribute lock the refresh call into both keydown handlers. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>
@assistant-ui/tapas a direct dependency because Studio now importsflushResourcesSyncdirectly.Fixes